fix(arrow-ipc): bound MessageReader allocations by actual stream bytes - #9869
Open
masumi-ryugo wants to merge 2 commits into
Open
fix(arrow-ipc): bound MessageReader allocations by actual stream bytes#9869masumi-ryugo wants to merge 2 commits into
masumi-ryugo wants to merge 2 commits into
Conversation
`MessageReader::maybe_next` decoded the on-wire `meta_len` (after the
round-1 check that rejects negative values) and the FlatBuffer message's
`bodyLength` and used both directly for up-front allocations:
self.buf.resize(meta_len, 0); // <— attacker-controlled
let mut buf = MutableBuffer::from_len_zeroed(message.bodyLength() as usize);
A 4-byte input — `00 1b 00 48` — claims a ~1.2 GiB metadata payload via
`meta_len = i32::from_le_bytes(...) = 0x4800_1b00`, driving a 1.2 GiB
`Vec::resize` before any short-read could fail. ~3×10^8 amplification
factor from input bytes to allocation; OOM-kills the process on a
2 GB-rss-limited fuzzer.
Read both metadata and body via incremental reads tied to the bytes
actually delivered by the underlying `Read`:
* metadata uses `take(meta_len).read_to_end(&mut self.buf)` followed
by an explicit length check;
* body is filled by a new `read_body_into_buffer` helper that
`extend_from_slice`s 64 KiB chunks into a `MutableBuffer`,
preserving the cache-line-aligned allocation that downstream
Arrow consumers rely on while keeping the high-water-mark
proportional to the bytes actually received.
Add `bodyLength` validation (`usize::try_from`) so a negative i64 is
surfaced as a `ParseError` instead of wrapping into a huge `usize`.
Add a regression test (`test_stream_reader_huge_meta_len_does_not_oom`)
that feeds the 4-byte fuzzer repro through `StreamReader::try_new` and
asserts a clean `Err`.
Found via cargo-fuzz libFuzzer harness wrapping `StreamReader::try_new`.
|
Independent confirmation from a fresh cargo-fuzz harness on this same code path — flagging this here because it's exactly the kind of evidence #5332 was set up to produce. Setup: I added a Pre-fix (current
Post-fix (this PR's code):
So this PR cleanly defuses the entire class of "single u32 in the header drives a multi-GB allocation" for |
This was referenced May 3, 2026
Contributor
|
run benchmark ipc_reader ipc_writer |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/arrow-ipc-body-length-bounded (c36a092) to fd86c75 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/arrow-ipc-body-length-bounded (c36a092) to fd86c75 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
This implementation seems to be quite a bit slower |
…caps Replace the chunked-read approach (which regressed StreamReader/no_validation/read_10 by ~86% per apache#9869 bench) with up-front bound checks against MAX_META_LEN (16 MiB) and MAX_BODY_LEN (2 GiB), then keep the original fast resize + read_exact path. Real Arrow IPC metadata (FlatBuffer schema descriptor) stays well under 1 MiB even for very wide tables; single record batches above 2 GiB are rare and exceed usize-on-32-bit anyway. Inputs above these limits reject with ParseError before any large allocation, defusing the 4-byte-header > multi-GB-resize OOM the PR was opened to fix. Local x86_64 bench (StreamReader/no_validation/read_10): main: 74.7 us (baseline) prev PR HEAD: 138.9 us (+86%) this commit: 74.3 us (within noise of main)
Contributor
Author
|
Thanks @alamb for the bench heads-up. The chunked-read approach was a poor trade against the steady-state cost — Reworked as up-front caps + the original fast path: Local x86_64 bench on
Caps are intentionally generous; happy to dial them tighter (e.g. metadata cap of 1 MiB) if you'd prefer a stricter ceiling. The 1.2 GiB-header regression test still passes. Disclosure: drafted with AI assistance, same caveats as the disclosure on #9884. |
Contributor
|
run benchmark ipc_reader ipc_writer |
alamb
reviewed
Jun 3, 2026
| } | ||
|
|
||
| /// Maximum bytes of IPC message metadata we will allocate up front from an | ||
| /// untrusted header. Real Arrow IPC metadata is a FlatBuffer schema descriptor |
Contributor
There was a problem hiding this comment.
How do we know that real IPC metadata is under 16MB? This seems like it could error on valid inputs
Comment on lines
+1803
to
+1804
| /// untrusted header. Single Arrow record batches above 2 GiB are rare in | ||
| /// practice and would push past `usize`-on-32-bit anyway. |
Contributor
There was a problem hiding this comment.
Even if they are rare in practice, that doesn't mean they are invalid. Won't this cap reject valid messages?
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/arrow-ipc-body-length-bounded (73847c5) to fd86c75 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/arrow-ipc-body-length-bounded (73847c5) to fd86c75 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
N/A — found via
cargo-fuzzlibFuzzer harness overStreamReader::try_new.Happy to file a tracking issue first if maintainers prefer.
Rationale for this change
MessageReader::maybe_nextdecodes the on-wiremeta_len(after theexisting check that rejects negative values) and the FlatBuffer message's
bodyLength, and uses both directly for up-front allocations:A 4-byte input —
00 1b 00 48— claims a ~1.2 GiB metadata payload viameta_len = i32::from_le_bytes(...) = 0x4800_1b00, driving a 1.2 GiBVec::resizebefore any short-read could fail. Roughly a 3×10⁸amplification factor from input bytes to allocation; OOM-kills the
process on a 2 GiB-rss-limited fuzzer or a memory-constrained service.
Per
SECURITY.mdthis is a bug, not a vulnerability (no RCE, noinformation disclosure — only availability), but it is reachable from
the public
StreamReader::try_newentrypoint and is the same shape ofbug that the recent
meta_len-negative fix addressed.What changes are included in this PR?
(&mut self.reader).take(meta_len).read_to_end(&mut self.buf)followed by an explicit length check, so the buffer grows as bytes
actually arrive instead of being eagerly resized to the (untrusted)
declared length.
read_body_into_bufferhelper that fills aMutableBufferin64 KiB chunks via
extend_from_slice. This preserves the cache-linealigned allocation that downstream Arrow consumers rely on, while
keeping the high-water-mark allocation proportional to the bytes
actually delivered by the underlying reader.
bodyLength(i64) viausize::try_from, surfacing anegative or out-of-range value as a
ParseErrorinstead of wrappingsilently into a huge
usizeon 64-bit and a different hugeusizeon 32-bit.
test_stream_reader_huge_meta_len_does_not_oom,that runs the 4-byte fuzzer repro through
StreamReader::try_newandasserts a clean
Err.Are these changes tested?
Yes — new unit test as above.
cargo test -p arrow-ipc --release(112 unit + 17 integration tests),
cargo clippy -p arrow-ipc --all-targets -- -D warnings, andcargo fmt --checkare all clean.Are there any user-facing changes?
Yes — malformed IPC streams that previously triggered a multi-GB
allocation now return an
ArrowError::ParseErrorearly. No behaviorchange for well-formed streams; allocation is still cache-line aligned
and the final buffer shape (
MutableBuffer) is unchanged.Reproducer
4 bytes:
Before this PR: a ~1.2 GiB allocation completes (or OOM-kills the
process under a memory limit) before
read_exactdiscovers there are0 bytes left and returns an EOF error.
After this PR:
Err(ArrowError::ParseError("Unexpected EOF reading 1207975168 bytes of message metadata, got 0")), with peak allocationon the order of the 64 KiB read chunk plus a small flatbuffer scratch.
Found via
cargo-fuzzlibFuzzer harness wrappingStreamReader::try_new.